Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enforce fine-grained form and flow validation #114

Merged
merged 5 commits into from
Mar 27, 2024
Merged

Enforce fine-grained form and flow validation #114

merged 5 commits into from
Mar 27, 2024

Conversation

ohltyler
Copy link
Member

@ohltyler ohltyler commented Mar 26, 2024 •
edited
Loading

Description

This PR improves the overall validation logic on the workflow editor:

  • moves the save/launch buttons from the global page to embedded within the editor, and adds a delete button at the global level. This is a placeholder and may be modified or removed in the future pending final UX design.
  • integrates the form state in each drag-and-drop component as part of the overall validation when clicking Save button. This is done by propagating callback fn onFormChange to the low-level form components, and performing form validation and submission when clicking Save button
  • adds callouts if something is invalid. separate messaging for form state vs. flow state is supported (final design/wording is TBD)
  • refactors the generation of the workspaceFlowState from workspace to parent component resizableWorkspace. This allows us to perform validation in a consistent way, and pass the same, updated workflow to all downstream components.
  • cleans up and simplifies saveWorkflow to perform only save/update

Additional changes:

  • nit change to always default to 'manage' tab on base Workflows page for consistency
  • remove placeholder Submit button since we have the logic available in the Save button now

Note that the form validation logic is complete - however, the flow validation logic is still in TODO state; for now, it always succeeds / is valid.

Screenshots:

Form validation:

Screenshot 2024-03-26 at 11 45 02 AM

Flow validation:

Screenshot 2024-03-26 at 11 44 26 AM

Demo videos:

Form validation integrated with Save button being disabled/enabled depending on unsaved changes. Note that on submitting / clicking Save, all invalid fields are highlighted appropriately, across all components. And, once everything is valid, the callout does not appear when saving.

screen-capture.18.webm

Flow validation integrated with Save button being disabled/enabled depending on unsaved changes. Ignore the callout as that logic is still a placeholder.

screen-capture.19.webm

Issues Resolved

Resolves #5

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

ohltyler added 3 commits March 25, 2024 11:47
@ohltyler
Migrate save/launch buttons; improve save disabled/enabled state
46d8bab 
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ohltyler
Propagate form change to parent save button
82fef7d 
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ohltyler
Add full form validation and flow validation + callouts
153b0e7 
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
ohltyler added 2 commits March 26, 2024 12:26
@ohltyler
Add few comments
6972f0b 
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ohltyler
Fix UT
439b05f 
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@codecov Codecov
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 21.39%. Comparing base (83835c2) to head (439b05f).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #114      +/-   ##
==========================================
+ Coverage   20.20%   21.39%   +1.19%     
==========================================
  Files          54       54              
  Lines         797      818      +21     
  Branches       74       79       +5     
==========================================
+ Hits          161      175      +14     
- Misses        630      639       +9     
+ Partials        6        4       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@amitgalitz
Copy link
Member

We will we use the backend validation here also in the future when saving?

@amitgalitz
Copy link
Member

Also will we have the first save be a create basically then update?

@ohltyler
Copy link
Member Author

We will we use the backend validation here also in the future when saving?

Definitely, the way I envision it is a 3-layer validation. First is basic form validation (no missing fields), second is flow validation (a DAG), and third would be backend validation. Whether we have it proactive (validation API) or reactive, we can support any way.

Also will we have the first save be a create basically then update?

Yes: https://github.com/opensearch-project/dashboards-flow-framework/blob/main/public/pages/workflow_detail/utils/utils.ts#L29:L33

amitgalitz
amitgalitz approved these changes Mar 27, 2024
View reviewed changes
Copy link
Member

@amitgalitz amitgalitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@amitgalitz
Copy link
Member

We will we use the backend validation here also in the future when saving?

Definitely, the way I envision it is a 3-layer validation. First is basic form validation (no missing fields), second is flow validation (a DAG), and third would be backend validation. Whether we have it proactive (validation API) or reactive, we can support any way.

Also will we have the first save be a create basically then update?

Yes: https://github.com/opensearch-project/dashboards-flow-framework/blob/main/public/pages/workflow_detail/utils/utils.ts#L29:L33

looks good to me, might want to revisit when we have those integrated how the save functionality will look like with the different validation, since as you commented save = create or update and launch is provision I assume. wonder if we should have differentiation for just quick create and launch too, or like we force to save and then launch, and obvious that the first save is equal to create

@ohltyler
Copy link
Member Author

We will we use the backend validation here also in the future when saving?

Definitely, the way I envision it is a 3-layer validation. First is basic form validation (no missing fields), second is flow validation (a DAG), and third would be backend validation. Whether we have it proactive (validation API) or reactive, we can support any way.

Also will we have the first save be a create basically then update?

Yes: https://github.com/opensearch-project/dashboards-flow-framework/blob/main/public/pages/workflow_detail/utils/utils.ts#L29:L33

looks good to me, might want to revisit when we have those integrated how the save functionality will look like with the different validation, since as you commented save = create or update and launch is provision I assume. wonder if we should have differentiation for just quick create and launch too, or like we force to save and then launch, and obvious that the first save is equal to create

Right this will all need to be revisited when things are more finalized, for now we have placeholders and TODOs in code. This is just incremental.

@ohltyler ohltyler merged commit 5231f09 into opensearch-project:main Mar 27, 2024
8 checks passed
@ohltyler ohltyler deleted the editor-buttons branch March 27, 2024 00:39
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 27, 2024
@ohltyler @github-actions
Enforce fine-grained form and flow validation (#114)
23ea277 
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
(cherry picked from commit 5231f09)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Mar 27, 2024
Merged
ohltyler added a commit that referenced this pull request Mar 27, 2024
@opensearch-trigger-bot @ohltyler
Enforce fine-grained form and flow validation (#114) (#117)
75ca998 
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
(cherry picked from commit 5231f09)

Co-authored-by: Tyler Ohlsen <ohltyler@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amitgalitz amitgalitz amitgalitz approved these changes

@minalsha minalsha minalsha approved these changes

@dbwiddis dbwiddis Awaiting requested review from dbwiddis dbwiddis is a code owner

@owaiskazi19 owaiskazi19 Awaiting requested review from owaiskazi19 owaiskazi19 is a code owner

@joshpalis joshpalis Awaiting requested review from joshpalis joshpalis is a code owner

@jackiehanyang jackiehanyang Awaiting requested review from jackiehanyang jackiehanyang is a code owner

Assignees
No one assigned
Labels
backport 2.x rapid workflow details workflow editor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Workflow editor] Fine-grained validation
3 participants
@ohltyler @amitgalitz @minalsha